lint: replace `flake8` with `ruff` check #1862

Borda · 2024-03-11T09:08:13Z

Resolves #1861

Borda · 2024-03-11T09:09:20Z

this is still missing

rst-roles =  # for flake8-RST-docstrings
    attr, class, func, meth, mod, obj, ref, term, var  # used by sphinx

and I would suggest replacing it with https://github.com/rstcheck/rstcheck#examples

Byron · 2024-03-11T10:07:51Z

Thanks a lot for tackling this! Maybe @EliahKagan can also have a look.

Borda · 2024-03-11T10:14:45Z

For the Literal issue we may need to import directly astral-sh/ruff#128 (comment)

Borda · 2024-03-11T10:28:20Z

@Byron could you pls help me to understand why the pytest is failing which seems to me as unrelated?

Byron · 2024-03-11T13:29:59Z

Happily!

I restarted the runs and it looks like they will be passing now - probably something wrong with the runner/bad luck. It's quite rare, fortunately, so I'd expect this to not happen again.

Borda · 2024-03-11T14:20:18Z

I restarted the runs and it looks like they will be passing now - probably something wrong with the runner/bad luck. It's quite rare, fortunately, so I'd expect this to not happen again.

thank you, there is still one more, it is possible it a flaky test?

FAILED test/test_installation.py::TestInstallation::test_installation - AssertionError: 0 != 2 : b'Processing /cygdrive/d/a/GitPython/GitPython\n'

Byron · 2024-03-11T14:25:33Z

Oh, I was declaring victory too early, sorry for that. This looks like a real failure then, but I don't know how that would be related. Would your changes affect the GitPython installation on Cygwin?

Borda · 2024-03-11T14:33:29Z

Would your changes affect the GitPython installation on Cygwin?

revisiting the change again and except the import Literal I did not not any code change

Byron · 2024-03-11T15:00:30Z

It looks real, but I don't know what it is about. Maybe ignore it for now until there is another PR running CI - if it's the same we know it's something very else.

EliahKagan · 2024-03-11T18:30:18Z

These lines from the GitHub Actions log reveal that this is due to a previously undetected bug in some code in GitPython's test suite. The bug is triggered by the (correct) changes to pyproject.toml here.

I have implemented a fix in #1864, which also gives a more detailed analysis.

EliahKagan

This looks good to me, and seems to cover all the bases.

(In particular, I agree with marking F401 unused-import as never automatically fixable. At least for now, some things would break if imports not listed in __all__ or directly used were removed.)

Together with #1864, the Cygwin test failure should go away. As mentioned there, I'm not sure of the best way to integrate them, but just about any way--including the simple way of just merging both, in either order--seems reasonable to me.

Byron · 2024-03-11T19:37:26Z

Thanks @EliahKagan for the review and your help fixing the test failure.
@Borda I recommend rebasing this PR onto main and it can be merged back.

Thanks everyone for the help and support!

Borda · 2024-03-11T19:54:30Z

I recommend rebasing this PR onto main and it can be merged back.

@Byron done :)

Byron

Thanks a lot for contributing and for your incredibly swift responses!

- `make lint` no longer lints without making changes. This has been the case since gitpython-developers#1862, because `--fix` is used. (I don't think anyone was really using `make lint`, but just in case, we should avoid saying it still never changes any files.) With gitpython-developers#1865, there will be more ways that files can be changed. - `black` is no longer used, having been replaced by `ruff`. This will be the case as of gitpython-developers#1865. This also reorganizes the "Specific tools" list, since they are all configured in pyproject.toml now (only flake8 was not before, and it was removed in favor of ruff in gitpython-developers#1862). In doing so, I've also added brief parenthesized phrases to characterize what each of these four tools is for, so readers don't have to look around as much to understand most of the tooling GitPython has set up.

This is to make it so simple `tox` usage has the expected property of leaving all source code files in the working tree unchanged. As noted in c66257e, linting how sometimes performs auto-fixes since gitpython-developers#1862, and the pre-commit command in tox.ini, which had also run `black --check`, will do even more file editing with gitpython-developers#1865. The bifurcation for black into separate mutating and non-mutating hooks, introduced in 5d8ddd9 (gitpython-developers#1693), is not carried over into Ruff autoformatting in gitpython-developers#1865. But also it: - Was not necessarily a good approach, and likely should not be preserved in any form. It is an unusual and unintuitive use of pre-commit. (It can be brought back if no better approach is found, though.) - Was done to avoid a situation where it was nontrivial to set up necessary dependencies for linting in the GitPython virtual environment itself, because flake8 and its various plugins would have to be installed. They were not listed in any existing or newly introduced extra (for example, they were not added to test-requirements.txt) in part in the hope that they would all be replaced by Ruff, which happened in gitpython-developers#1862. - Already does not achieve its goal since gitpython-developers#1862, since it was (probably rightly) not extended to Ruff linting to use/omit --fix. Now that Ruff is being used, people can run `pip install ruff` in a virtual environment, then run the `ruff` command however they like. This takes the place of multiple tools and plugins. This commit *avoids* doing any of the following, even though it may be useful to do them later: - This does not give specific instructions in the readme for installing and running ruff (and c66257e before this also omits that). This can be added later and the best way to document it may depend on some other upcoming decisions (see below). - This does not add ruff to the test extra or as any other kind of extra or optional dependency. Although the test extra currently contains some packages not used for running unit tests, such as pre-commit and mypy, adding Ruff will cause installation to take a long time and/or or fail on some platforms like Cygwin where Ruff has to be built from (Rust) source code. This can be solved properly by reorganizing the extras, but that is likely to wait until they are expressed completely inside pyproject.toml rather than one per requirements file (see discussion in comments in gitpython-developers#1716 for general information about possible forthcoming changes in how the project is defined). - This does not update tox.ini to run ruff directly, which could be done by splitting the current lint tox environment into two or three environments for Python linting, Python autoformatting, and the miscellaneous other tasks performed by pre-commit hooks, only the latter of which would use the pre-commit command. Whether and how this should be done may depend on other forthcoming changes. - This does not remove or update the Makefile "lint" target. That target should perhaps not have been added, and was always meant to be improved/replaced, since everything else in the top-level Makefile is for building and publishing releases. See 5d15063 (gitpython-developers#1693). This is likewise not done now since it may depend on as-yet unmerged changes and tooling decisions not yet made. It should be feasible to do together when further updating tox.ini. - This does not update tox.ini, Makefile, or the lint.yml GitHub Actions workflow to omit the manual hook-stage, which will be unused as of gitpython-developers#1865. This would complicate integration of changes, and once it is known that it won't be needed, it can removed. The situation with the tox "lint" environment is thus now similar to that of the tox "html" environment when it was added in e6ec6c8 (gitpython-developers#1667), until it was improved in f094909 (gitpython-developers#1693) to run with proper isolation.

This also reorganizes the "Specific tools" list, since they are all configured in pyproject.toml now (only flake8 was not before, and it was removed in favor of ruff in gitpython-developers#1862). In doing so, I've also added brief parenthesized phrases to characterize what each of these four tools is for, so readers don't have to look around as much to understand most of the tooling GitPython has set up.

This is to make it so simple `tox` usage has the expected property of leaving all source code files in the working tree unchanged. Linting how sometimes performs auto-fixes since gitpython-developers#1862, and the pre-commit command in tox.ini, which had also run `black --check`, will do even more file editing due to the changes in gitpython-developers#1865. The bifurcation for black into separate mutating and non-mutating hooks, introduced in 5d8ddd9 (gitpython-developers#1693), was not carried over into Ruff autoformatting in gitpython-developers#1865. But also it: - Was not necessarily a good approach, and likely should not be preserved in any form. It was an unusual and unintuitive use of pre-commit. (It can be brought back if no better approach is found, though.) - Was done to avoid a situation where it was nontrivial to set up necessary dependencies for linting in the GitPython virtual environment itself, because flake8 and its various plugins would have to be installed. They were not listed in any existing or newly introduced extra (for example, they were not added to test-requirements.txt) in part in the hope that they would all be replaced by Ruff, which happened in gitpython-developers#1862. - Already did not achieve its goal as of gitpython-developers#1862, since it was (probably rightly) not extended to Ruff linting to use/omit --fix. Now that Ruff is being used, people can run `pip install ruff` in a virtual environment, then run the `ruff` command however they like. This takes the place of multiple tools and plugins. The situation with the tox "lint" environment is thus now similar to that of the tox "html" environment when it was added in e6ec6c8 (gitpython-developers#1667), until it was improved in f094909 (gitpython-developers#1693) to run with proper isolation.

Although it seems likely that the requirements-dev.txt file will be removed when the project definition is made declarative (discussed in gitpython-developers#1716 comments), if not before, for now it exists and might be in use, so this updates it with tools that are currently used but not listed in any extras or other requirements files: - ruff: This has replaced flake8 and its plugins (gitpython-developers#1862) as well as black (gitpython-developers#1865). Currently there is no separate extra for tooling that is not part of unit testing, with some such tools listed in test-requirements.txt. The `ruff` package belongs here rather than there for now because it should not be installed just to run unit tests, since Ruff has to be built from source on some rarer platforms like Cygwin, which would take a long time and/or fail. - shellcheck: The PyPI package for this is a convenience for installing it in projects that are already using pip (shellcheck is neither written in Python nor a tool to scan Python code). It installs pre-built binaries, which are not available for all platforms. These packages remain listed: - pytest-icdiff: This seems not to have been promoted to be in the test-requirements.txt file and the `test` extra because it does not work as well for diffs from tests that the pytest runner runs but that are written for the unittest framework. - pytest-profiling [commented out]: I am not sure what the status of this is, perhaps it has just not been experimented with enough to know if it would be useful for profiling in GitPython. This requirements-dev.txt file has a few limitations that suggest it should be removed altogether sometime soon: - It is not updated regularly. - It is not always clear why something is there. Originally I believe it was for tools where the desire to use the tool was established but the tool did not yet work or worked but performed checks for which code had to be fixed. That purpose has drifted. - It uses a different naming convention from the test-requirements.txt file in active use. - It cannot be readily used to create an extra in the current project definition in setup.py because the simple parsing done there will not recognize the `-r` lines and will not skip the comments, and neither enhancement should be done in setup.py since that would move things farther away from a declarative project definition. - It will naturally go away when the project definition is made declarative, since it will then be feasible to define as many extras as desired without proliferating separate requirements files (while still allowing their contents to be statically available to tools). Since it may go away soon and is not regularly updated, I have kept the explanations for why particular packages are there out of it. But as long as it exists it may as well list the tools that really are being used yet are not explicitly listed as dependencies.

The conditional imports of Literal from either typing or typing_extensions have to be done with if-else on the Python version rather than with try-except, or it is a static type error. This makes that change, checking sys.version_info. (See discussion in gitpython-developers#1861 and gitpython-developers#1862 for broader context and background on why this logic, before and after this change, is repeated across multiple modules.) This also reorders/regroups imports for consistency in some places, especially where a new import of the sys module (for version_info) would otherwise exacerbate inconsistency. Since the merge commit 0b99041, the number of mypy errors had increased from 5 to 10. This fixes all the new mypy errors, so the count is back to 5.

Borda marked this pull request as ready for review March 11, 2024 10:18

EliahKagan mentioned this pull request Mar 11, 2024

Use upgraded pip in test fixture virtual environment #1864

Merged

EliahKagan approved these changes Mar 11, 2024

View reviewed changes

EliahKagan mentioned this pull request Mar 11, 2024

Improve static typing and docstrings related to git object types #1859

Merged

Borda added 3 commits March 11, 2024 20:53

lint: replace flake8 with ruff check

0b7f159

fixing lints / noqa

aa9298a

try: from typing import Literal

5f889c4

Borda force-pushed the lint/ruff branch from a083e81 to 5f889c4 Compare March 11, 2024 19:54

Byron approved these changes Mar 11, 2024

View reviewed changes

Byron merged commit edb8d26 into gitpython-developers:main Mar 11, 2024
26 checks passed

Byron changed the title ~~lint: replace flake8 with ruff check~~ 📣lint: replace flake8 with ruff check 📣 Mar 11, 2024

Borda deleted the lint/ruff branch March 11, 2024 20:29

Borda mentioned this pull request Mar 11, 2024

lint: switch Black with ruff-format #1865

Merged

EliahKagan mentioned this pull request Mar 12, 2024

Update readme and tox.ini for recent tooling changes #1868

Merged

EliahKagan mentioned this pull request Mar 15, 2024

NullHandler added to all but git.repo.base logger #1806

Closed

Byron changed the title ~~📣lint: replace flake8 with ruff check 📣~~ lint: replace flake8 with ruff check Mar 15, 2024

EliahKagan mentioned this pull request Mar 19, 2024

Replace all wildcard imports with explicit imports #1880

Merged

EliahKagan mentioned this pull request Mar 31, 2024

Detailed xfail output sometimes distracts from unexpected failures #1891

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lint: replace `flake8` with `ruff` check #1862

lint: replace `flake8` with `ruff` check #1862

Borda commented Mar 11, 2024

Borda commented Mar 11, 2024

Byron commented Mar 11, 2024

Borda commented Mar 11, 2024

Borda commented Mar 11, 2024

Byron commented Mar 11, 2024

Borda commented Mar 11, 2024

Byron commented Mar 11, 2024

Borda commented Mar 11, 2024

Byron commented Mar 11, 2024

EliahKagan commented Mar 11, 2024

EliahKagan left a comment

Byron commented Mar 11, 2024

Borda commented Mar 11, 2024 •

edited

Loading

Byron left a comment

lint: replace flake8 with ruff check #1862

lint: replace flake8 with ruff check #1862

Conversation

Borda commented Mar 11, 2024

Borda commented Mar 11, 2024

Byron commented Mar 11, 2024

Borda commented Mar 11, 2024

Borda commented Mar 11, 2024

Byron commented Mar 11, 2024

Borda commented Mar 11, 2024

Byron commented Mar 11, 2024

Borda commented Mar 11, 2024

Byron commented Mar 11, 2024

EliahKagan commented Mar 11, 2024

EliahKagan left a comment

Choose a reason for hiding this comment

Byron commented Mar 11, 2024

Borda commented Mar 11, 2024 • edited Loading

Byron left a comment

Choose a reason for hiding this comment

lint: replace `flake8` with `ruff` check #1862

lint: replace `flake8` with `ruff` check #1862

Borda commented Mar 11, 2024 •

edited

Loading